Skip to content

[fix] Fix hostname verification #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jan 17, 2023
Merged

[fix] Fix hostname verification #126

merged 5 commits into from
Jan 17, 2023

Conversation

@izumo27
Copy link
Contributor

@izumo27 izumo27 commented Nov 25, 2022

Motivation

If ValidateHostName is set to true, handshake always fails.

INFO  [] ClientConnection:375 | [ -> ] Connected to broker
ERROR [] ClientConnection:463 | [ -> ] Handshake failed: certificate verify failed
INFO  [] ClientConnection:1560 | [ -> ] Connection closed

Modifications

  • Verify serviceUrl.host(), not physicalAddress.
    • physicalAddress is serviceUrl, which contains protocol (e.g. pulsar+ssl) and port number.
  • Use ssl::stream::set_verify_callback instead of ssl::context::set_verify_callback.
    • Verification should work with ssl::context::set_verify_callback, but somehow it doesn't work.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@BewareMyPower BewareMyPower added bug Something isn't working release/3.0 labels Nov 25, 2022
merlimat
merlimat reviewed Nov 28, 2022
View reviewed changes
@izumo27 izumo27 force-pushed the fix_hostname_verification branch from 7a6cd3b to a38e41b Compare December 1, 2022 08:10
@izumo27
Copy link
Contributor Author

izumo27 commented Dec 1, 2022

Rebased master

@izumo27
Copy link
Contributor Author

izumo27 commented Dec 14, 2022

@merlimat PTAL, thanks

BewareMyPower
BewareMyPower requested changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After I reverted your changes in ClientConnection.cc, these tests could still pass. Could you explain it?

image

$ ./tests/pulsar-tests --gtest_filter='AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile'
Note: Google Test filter = AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AuthPluginTest
[ RUN      ] AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile
2022-12-19 19:55:54.972 INFO  [140386200329920] ClientConnection:190 | [<none> -> pulsar+ssl://localhost:6651] Create ClientConnection, timeout=10000
2022-12-19 19:55:54.973 INFO  [140386200329920] ConnectionPool:97 | Created connection for pulsar+ssl://localhost:6651
2022-12-19 19:55:54.974 INFO  [140386197206784] ClientConnection:387 | [127.0.0.1:50090 -> 127.0.0.1:6651] Connected to broker
2022-12-19 19:55:54.982 ERROR [140386197206784] ClientConnection:487 | [127.0.0.1:50090 -> 127.0.0.1:6651] Handshake failed: certificate verify failed
2022-12-19 19:55:54.982 INFO  [140386197206784] ClientConnection:1599 | [127.0.0.1:50090 -> 127.0.0.1:6651] Connection closed with ConnectError
2022-12-19 19:55:54.982 ERROR [140386197206784] ClientImpl:183 | Error Checking/Getting Partition Metadata while creating producer on persistent://private/auth/testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile -- ConnectError
2022-12-19 19:55:54.982 INFO  [140386197206784] ClientConnection:268 | [127.0.0.1:50090 -> 127.0.0.1:6651] Destroyed connection
[       OK ] AuthPluginTest.testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile (13 ms)

@izumo27
Copy link
Contributor Author

izumo27 commented Dec 19, 2022 •
edited
Loading

After I reverted your changes in ClientConnection.cc, these tests could still pass.

testTlsDetectPulsarSslWithHostNameValidationMissingCertsFile tests whether the result of createProducer is a connection error when certsFile is missing. Hostname verification of master always fails, so this test could still pass.

This PR fixes testTlsDetectPulsarSslWithHostNameValidation. When certsFile is set corretly, the result of createProducer should be ok, not a connection error.
https://github.com/apache/pulsar-client-cpp/blob/v3.1.0/tests/AuthPluginTest.cc#L152
https://github.com/apache/pulsar-client-cpp/pull/126/files#diff-43a1460bfe2216170e3d7a5f634e8d0422b78946b018f81118dc275cbba2d04fR152

RobertIndie
RobertIndie reviewed Dec 29, 2022
View reviewed changes
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a test case that demonstrates the failed case of hostname verification? Like here: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticationTlsHostnameVerificationTest.java#L140

@RobertIndie RobertIndie added this to the 3.2.0 milestone Dec 29, 2022
@izumo27
Copy link
Contributor Author

izumo27 commented Jan 5, 2023

@RobertIndie

Could you also add a test case that demonstrates the failed case of hostname verification?

The test requires changing configurations of a broker, but mocking is difficult.
I don't think it is a good idea to set up another standalone cluster.
Do you have any ideas?

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jan 5, 2023 •
edited
Loading

You can change the current config test-conf/standalone-ssl.conf directly. If it can be configured at namespace level, you can create a separated namespace for tests.

If it requires changing configurations at broker level and it will affect other tests, I think setting up another standalone is okay.

@izumo27 izumo27 mentioned this pull request Jan 11, 2023
Open
2 tasks
@izumo27
Copy link
Contributor Author

izumo27 commented Jan 13, 2023

I added tests for invalid broker using another standalone.

@BewareMyPower
Copy link
Contributor

The fix LGTM, but could you describe how to generate this certificates? It would not block this PR but I think it could be improved to make tests clear. It seems that when hn-verification/cacert.pem is used, setValidateHostName must be set true.

@izumo27
Copy link
Contributor Author

izumo27 commented Jan 17, 2023

could you describe how to generate this certificates?

The certificates I added for man-in-the-middle attacks is the same as the certificates in pulsar repository.
https://github.com/apache/pulsar/tree/v2.11.0/pulsar-broker/src/test/resources/authentication/tls/hn-verification
This is also true for other existing certificates in pulsar-client-cpp repository.

@BewareMyPower BewareMyPower merged commit 3202bcd into apache:main Jan 17, 2023
merlimat pushed a commit that referenced this pull request Jan 19, 2023
@izumo27 @merlimat
[fix] Fix hostname verification (#126)
9350d1b 
### Motivation

If `ValidateHostName` is set to true, handshake always fails.
```
INFO  [] ClientConnection:375 | [ -> ] Connected to broker
ERROR [] ClientConnection:463 | [ -> ] Handshake failed: certificate verify failed
INFO  [] ClientConnection:1560 | [ -> ] Connection closed
```

### Modifications

- Verify `serviceUrl.host()`, not `physicalAddress`.
  - `physicalAddress` is serviceUrl, which contains protocol (e.g. pulsar+ssl) and port number.
- Use `ssl::stream::set_verify_callback` instead of `ssl::context::set_verify_callback`.
  - Verification should work with `ssl::context::set_verify_callback`, but somehow it doesn't work.

Co-authored-by: hoguni <hoguni@yahoo-corp.jp>
@BewareMyPower BewareMyPower mentioned this pull request Apr 14, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merlimat merlimat merlimat left review comments

@RobertIndie RobertIndie RobertIndie left review comments

@BewareMyPower BewareMyPower BewareMyPower approved these changes

Assignees

@izumo27 izumo27

Labels

bug Something isn't working release/3.1.1

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@izumo27 @BewareMyPower @merlimat @RobertIndie